-
Notifications
You must be signed in to change notification settings - Fork 23
feat: show remaining CDN & cache-miss egress #538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 164bc70 | Commit Preview URL Branch Preview URL |
Jan 14 2026, 11:27 AM |
apps/synapse-playground/src/components/warm-storage/data-sets-section.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
|
This seems fine to me and I appreciate the AGENTS.md edits but I'm going to leave it to @hugomrdias to review |
- Change format to "Egress remaining: X GiB delivery · Y GiB cache-miss" - Remove FolderSync icon, keep Globe icon with tooltip "This data set is using CDN" - Use binary units (KiB, MiB, GiB, TiB) instead of decimal units Co-Authored-By: Claude Code <[email protected]> Signed-off-by: Miroslav Bajtoš <[email protected]>
|
I addressed the review comments, this PR is ready for another round of reviews. 🙏🏻 |
pyropy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
| export function formatBytes(bytes: bigint | number): string { | ||
| const num = typeof bytes === 'bigint' ? Number(bytes) : bytes | ||
| if (num === 0) return '0 B' | ||
|
|
||
| const units = ['B', 'KiB', 'MiB', 'GiB', 'TiB'] | ||
| const k = 1024 | ||
| const i = Math.floor(Math.log(num) / Math.log(k)) | ||
| const value = num / k ** i | ||
|
|
||
| return `${value.toFixed(2).replace(/\.?0+$/, '')} ${units[i]}` | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot use pretty-bytes:
Note that it uses base-10 (e.g. kilobyte). Read about the difference between kilobyte and kibibyte.
FilBeam's pricing uses multiples of 1024, not base-10.
I could use byte-size (32 kB unpacked) or bytes-iec (23 kB unpacked), but do we really need to add so much weight to our dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep the current implementation, then I propose moving the helper to synapse-core.
Any objections?
| </TooltipContent> | ||
| </Tooltip> | ||
| <span className="flex items-center gap-1 text-sm text-muted-foreground"> | ||
| <Tooltip> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would make a custom tooltip component wrapping the globe icon with all the cdn info, using a hook to fetch the data.
check the other hooks that use tanstack query to wrap fetchDataSetEgressQuota
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make a custom tooltip component wrapping the globe icon with all the cdn info, using a hook to fetch the data.
Since we are fetching isCDN flag in a different network request than the egress quotas info, I feel it's better to keep the current globe & tooltip as-is, and let the new component handle only the part about egress quotas.
That's what I implemented in the new version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH, I can see how it would make sense to move the entire <span> into a new component, growing EgressQuotaDisplay to something like CdnDetails.
{dataSet.cdn && <CdnDetails dataSetId={dataSet.dataSetId}/>}Is that what you had in mind?
Can you suggest a better name for this new component?
- Add filbeam module to synapse-core with getDataSetStats function - Use iso-web/http request lib with proper error handling - Add GetDataSetStatsError class for FilBeam API errors - Add parseBigInt validation for API response values - Create useEgressQuota hook in synapse-react - Remove egress quota fetching from useDataSets hook - Update playground to use new useEgressQuota hook Co-Authored-By: Claude Code <[email protected]> Signed-off-by: Miroslav Bajtoš <[email protected]>
Add unit tests for getStatsBaseUrl and validateStatsResponse functions. Export validateStatsResponse to enable direct testing without HTTP mocking. Also update AGENTS.md with testing guidelines: - Use assert.deepStrictEqual for object comparisons - Use parameterized tests with descriptive names Co-Authored-By: Claude Code <[email protected]> Signed-off-by: Miroslav Bajtoš <[email protected]>
just in case Signed-off-by: Miroslav Bajtoš <[email protected]>
|
@hugomrdias I addressed your comments, PTAL again. |
| @@ -1,9 +1,9 @@ | |||
| import type { DataSetWithPieces, UseProvidersResult } from '@filoz/synapse-react' | |||
| import { useDeletePiece } from '@filoz/synapse-react' | |||
| import { useDeletePiece, useEgressQuota } from '@filoz/synapse-react' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we find a better name for this hook? E.g. useFetchEgressQuotas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is a fetch egress quota? I don't think I've heard the term before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed name is a combination of the "use" prefix, followed by a verb ("to fetch"), followed by the object ("egress quota").
Similarly to how `useDeletePiece" is a combination of the "use" prefix, the verb "to delete", and the object "piece".
How about useGetEgressQuotas - is that easier to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see! I thought this was about a type of egress quota. I don't think we need a verb here, like there's also useState, and not useSetAndRetrieveState. I'd say useDeletePiece is a bit odd, but there the verb makes sense, since it's only one particular piece action.
| export function formatBytes(bytes: bigint | number): string { | ||
| const num = typeof bytes === 'bigint' ? Number(bytes) : bytes | ||
| if (num === 0) return '0 B' | ||
|
|
||
| const units = ['B', 'KiB', 'MiB', 'GiB', 'TiB'] | ||
| const k = 1024 | ||
| const i = Math.floor(Math.log(num) / Math.log(k)) | ||
| const value = num / k ** i | ||
|
|
||
| return `${value.toFixed(2).replace(/\.?0+$/, '')} ${units[i]}` | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep the current implementation, then I propose moving the helper to synapse-core.
Any objections?
|
|
||
| return useQuery({ | ||
| ...props.query, | ||
| queryKey: ['synapse-filbeam-egress-quota', chainId, dataSetId?.toString()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use the plural "quotas"?
| queryKey: ['synapse-filbeam-egress-quota', chainId, dataSetId?.toString()], | |
| queryKey: ['synapse-filbeam-egress-quotas', chainId, dataSetId?.toString()], |
Update the demo app to show remaining egress allowances for each data set where the FilBeam service is enabled.
Screenshot showing the new version in practice:
version 2 using FolderSync icon
version 1 using RefreshCw icon